Move notification handler registrations to capabilities#207
Conversation
There was a problem hiding this comment.
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tests/ModelContextProtocol.Tests/SseIntegrationTests.cs:233
- The square bracket initializer syntax used for NotificationHandlers may not be standard C#. Consider replacing it with a conventional collection initializer (e.g., 'new [] { ... }' or 'new List<KeyValuePair<string, Func<JsonRpcNotification, Task>>>() { ... }') to ensure compatibility.
NotificationHandlers = [new("test/notification", (args) =>
halter73
left a comment
halter73
left a comment
There was a problem hiding this comment.
While I think it might be useful to be able to add handlers mid-session, the fact you couldn't remove them made it half-baked regardless. And we can always add that capability back if there's a real need.
Someone can also do it on their own, just by adding a handler that reads from any collection they want to subsequently mutate. That single handler will be invoked and can effectively proxy the call to any number of handlers elsewhere. |
Currently request handlers are set on the capability objects, but notification handlers are set after construction via an AddNotificationHandler method on the IMcpEndpoint interface. This moves handler specification to be at construction as well. This makes it more consistent with request handlers, simplifies the IMcpEndpoint interface to just be about message sending, and avoids a concurrency bug that could occur if someone tried to add a handler while the endpoint was processing notifications.
stephentoub
force-pushed
the
movenotificationregistration
branch
from
c57d65d to
824d01d
Compare
|
I've been thinking more about this. This is a good direction over what was there before, which prohibited removal of added handlers, limiting their use. But if we instead allow removal, it becomes much more useful. Like with CancellationToken.Register, it the enables transiently listening for certain notifications, so for example you could create a progress token, hook up a notification handler that pays attention to only that token, then start the operation using that token, and then after the operation completes, remove that handler. It's hard to imagine how this could be achieved in a general fashion against an arbitrary IMcpClient/Server in other ways. Given that, I'm going to move back in that direction, putting it back onto the interface, but with a shape like CT.Register that enables scoped removal. cc: @halter73, @eiriktsarpalis, @PederHP |
| <!-- Without this, tests are currently not showing results until all tests complete | ||
| https://xunit.net/docs/getting-started/v3/microsoft-testing-platform | ||
| --> | ||
| <DisableTestingPlatformServerCapability>true</DisableTestingPlatformServerCapability> |
There was a problem hiding this comment.
@stephentoub Is this still happening for you?
If I create a new xunit.v3 project with the following class:
public class UnitTest1
{
[Fact]
public void Test1()
{
Thread.Sleep(10000);
}
[Fact]
public void Test2()
{
Thread.Sleep(10000);
}
}I get the test status updated after each test.
Is it some glitch that's not consistent? Is it already fixed? Maybe happens in specific scenarios?
I would like to know more about your situation.
jeffhandley
added
the
breaking-change
|
Adding the |
Currently request handlers are set on the capability objects, but notification handlers are set after construction via an AddNotificationHandler method on the IMcpEndpoint interface. This moves handler specification to be at construction as well. This makes it more consistent with request handlers, simplifies the IMcpEndpoint interface to just be about message sending, and avoids a concurrency bug that could occur if someone tried to add a handler while the endpoint was processing notifications.